Reviewed 11 of 13 files at r1, all commit messages.
Reviewable status: 11 of 13 files reviewed, 2 unresolved discussions

mullvad-ios/src/api_client/retry_strategy.rs line 49 at r1 (raw file):

#[no_mangle]
pub unsafe extern "C" fn mullvad_api_retry_strategy_never() -> SwiftRetryStrategy {

We should have # Safety warnings for all these functions marked unsafe
mullvad_api_get_addresses has a good example of what these should cover

ios/MullvadREST/RetryStrategy/RetryStrategy.swift line 25 at r1 (raw file):

        }

        /// The return value of this function *must* be passed to a Rust FFI funciton that will consume it, otherwise it will leak.

nit
funciton

Overall looks good, we'll have to make sure that the tests in RetryStrategyTests use the rust iterators if possible

Reviewable status: 11 of 13 files reviewed, 2 unresolved discussions (waiting on @SteffenErn)

Reviewed 13 of 13 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @buggmagnet and @SteffenErn)

mullvad-ios/src/api_client/retry_strategy.rs line 49 at r1 (raw file):

+1

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @buggmagnet and @SteffenErn)

mullvad-ios/src/api_client/retry_strategy.rs line 49 at r1 (raw file):

Under what circumstances would it be unsafe to call this function?

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @buggmagnet and @SteffenErn)

Reviewed 11 of 13 files at r1, 1 of 2 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @buggmagnet and @SteffenErn)

Reviewed 1 of 13 files at r1, 1 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @buggmagnet and @SteffenErn)

Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @buggmagnet and @SteffenErn)

Reviewed all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @buggmagnet and @SteffenErn)

Reviewed 1 of 3 files at r3.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @buggmagnet and @SteffenErn)

mullvad-ios/src/api_client/retry_strategy.rs line 48 at r4 (raw file):

/// Creates a retry strategy that never retries after failure.
/// The result needs to be consumed.

Consumed by what? I think this could be elaborated.

Reviewed 1 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @pinkisemils, @rablador, and @SteffenErn)

mullvad-ios/src/api_client/retry_strategy.rs line 49 at r1 (raw file):
Technically none of these are inherently unsafe, but to quote the rust documentation

All FFI (Foreign Function Interface) functions are unsafe to call because the other language can do arbitrary operations that the Rust compiler can't check.

We can just state in the # Safety lines that the functions are safe to call, as we do all across our existing Rust codebase.

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @buggmagnet and @SteffenErn)

mullvad-ios/src/api_client/retry_strategy.rs line 49 at r1 (raw file):

What is the safety hazard in calling this function? I am asking because I want to understand what are you trying to get at.

As far as I can tell, this call is always safe, it will panic and crash badly only in the case where it fails to allocate memory or there is not enough stack space to call it safely. If we are going to document those failure modes, then we can presume that all function calls are not safe. Am I missing something?

Reviewed 2 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @buggmagnet and @SteffenErn)

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @buggmagnet and @SteffenErn)

mullvad-ios/src/api_client/retry_strategy.rs line 49 at r1 (raw file):

Maybe the correct thing to do here is to not mark these functions as unsafe instead :)

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @pinkisemils, @rablador, and @SteffenErn)

mullvad-ios/src/api_client/retry_strategy.rs line 49 at r1 (raw file):

Yes if we can do that and have the compiler be happy about it, that would be my preferred option too

Reviewed 2 of 2 files at r5, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @pinkisemils and @SteffenErn)

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @SteffenErn)